-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Release OpenMP resources in blas_thread_shutdown #4080
base: develop
Are you sure you want to change the base?
Conversation
Thanks - I have not had time to review in detail, but perhaps it might be better to put the OpenMP version test in the c_check script rather than getarch |
570d6ba
to
645eabe
Compare
Thanks for your reply!
What do you think about skipping the test using the same mechanism that
currently skips it on builds without double precision support? It seems
to make for a smaller diff.
I could definitely use c_check if you prefer it for architectural reasons
and the omp_pause_resource_all() idea turns out to be sound. (I didn't
originally notice that you have a mechanism for checking preprocessor
symbols.)
|
Assume that the ctest3.c program successfully compiling and linking means that the function is available.
OpenMP 5.0 introduced the function omp_pause_resource_all that instructs the runtime to "relinquish resources used by OpenMP on all devices". In practice, these resources include the locks that would otherwise trip up the runtime after a fork(). Releasing these resources in a function called by pthread_atfork() makes it possible for the child process to continue functioning after the runtime automatically re-acquires its resources. Thread safety: blas_thread_shutdown doesn't check whether there are other BLAS operations running in parallel, so this isn't any less safe than before with respect to OpenBLAS function calls. On the other hand, if there are other OpenMP operations in progress, asking the runtime to pause may result in unspecified behaviour. A hard pause is allowed to deallocate threadprivate variables too.
In addition to testing fork safety on non-OpenMP builds, test it when omp_pause_resource_all() is available to release the locks.
645eabe
to
806073c
Compare
Sorry, must admit that I lost track of this... I'm not entirely sure of the binding region for this operation, what happens if this gets called while OpenBLAS is used by a program that uses multiple OpenMP threads itself (would there be a need for an |
Good question. The binding region for Unfortunately, OpenMP provides no other API to stop and restart the worker threads, and this function must be called before Experimentally, the following program seems to behave fine with
|
if (unlikely(blas_server_avail == 0)) blas_thread_init(); |
...and fall back to sequential operation instead of calling into OpenMP runtime and potentially hanging?
OpenMP 5.0 gained an
omp_pause_resource_all
function designed to release the locks beforefork
ing the process. The parameters are described in theomp_pause_resource
function. Using this function makes it possible for OpenMP builds of OpenBLAS to passfork
safety tests.An important detail is that there exist OpenMP implementations that only claim OpenMP 4.5 compatibility (i.e. they
#define _OPENMP 201511
) while they do have theomp_pause_resource_all
. We currently don't have a way to useomp_pause_resource_all
with such implementations (e.g. GCC 10 on Debian 11).The first commit adds the
omp_pause_resource_all
call toblas_thread_shutdown
. You may want not to merge this if you'd likeblas_thread_shutdown
to be safe to call while other OpenMP operations are in progress.The second commit adds a convoluted way to test
fork
safety of the resulting build on OpenMP ≥ 5.0 in addition to non-OpenMP builds. I'd be glad to see a better way of testing for OpenMP ≥ 5.0 in a Makefile.